Enhance testing strategy and add regression tests for ggRandomForests#67
Conversation
- Introduced a detailed code review document outlining testing strategy, identified gaps, and proposed actions for improvement. - Created a release checklist for version 2.7.0, summarizing changes and ensuring all necessary steps are followed before submission. - Added unit tests for kaplan(), nelson(), and bootstrap_survival() functions to ensure proper functionality and output structure. - Implemented visual regression tests using vdiffr for key plotting functions to ensure consistent visual output across changes.
- Skip vdiffr visual-regression tests in CI until reference SVGs are committed. - Adjust lintr configuration by removing comments for clarity. - Enhance plot.gg_rfsrc function to include a notch parameter for boxplots. - Mark a utility function as internal in the documentation.
…ting configuration
…meter in plot function
…base R; add linting comments for clarity
…e_linter; improve code formatting in various R scripts and tests
…update comments for clarity
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #67 +/- ##
==========================================
+ Coverage 83.37% 84.73% +1.36%
==========================================
Files 23 24 +1
Lines 1816 1801 -15
==========================================
+ Hits 1514 1526 +12
+ Misses 302 275 -27 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR prepares ggRandomForests v2.7.0 with a focus on strengthening test coverage and preventing regressions in plotting/data-extraction behavior, alongside CI/lint tightening and documentation refreshes.
Changes:
- Migrates the test suite to testthat 3.x conventions and adds new regression/unit tests (including survival helpers and ROC defaults).
- Updates multiple plotting/data functions to fix ggplot2 aesthetic mappings, improve robustness, and modernize tidyr usage (pivot_longer).
- Tightens CI workflows (lint failures, warnings-as-errors, Codecov action updates) and updates docs/metadata for the 2.7.0 release.
Reviewed changes
Copilot reviewed 70 out of 71 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/testthat/test_varpro_feature_names.R | Removes deprecated context() call. |
| tests/testthat/test_surv_partial.R | Adds additional tests and a shared rfsrc fixture for surv_partial.rfsrc. |
| tests/testthat/test_snapshots.R | Adds vdiffr visual regression tests guarded by env checks. |
| tests/testthat/test_shift.R | Updates expectations to testthat 3 style (expect_identical). |
| tests/testthat/test_randomForest_helpers.R | Updates type expectations to testthat 3 style. |
| tests/testthat/test_quantile_pts.R | Updates expectations and minor whitespace cleanup. |
| tests/testthat/test_kaplan_nelson.R | Adds comprehensive unit tests for kaplan/nelson/bootstrap survival helpers. |
| tests/testthat/test_ggrandomforests_news.R | Removes deprecated context() call. |
| tests/testthat/test_gg_vimp.R | Modernizes assertions; adds regression coverage for randomForest importance column naming. |
| tests/testthat/test_gg_variable.R | Modernizes assertions; adds more survival/regression/classification plotting coverage. |
| tests/testthat/test_gg_survival.R | Modernizes assertions for gg_survival behavior. |
| tests/testthat/test_gg_roc.R | Modernizes assertions; adds regression test for oob default; fixes method name typo in tests. |
| tests/testthat/test_gg_rfsrc.R | Modernizes assertions; expands survival and notch coverage. |
| tests/testthat/test_gg_partialpro.R | Removes deprecated context() call. |
| tests/testthat/test_gg_partial.R | Updates error-path tests to use expect_error() correctly. |
| tests/testthat/test_gg_error.R | Modernizes assertions; adds direct plot.gg_error() branch coverage. |
| release-checklist-v2.7.0.md | Adds an internal release checklist for v2.7.0. |
| man/surv_partial.rfsrc.Rd | Expands and clarifies documentation, return structure, and seealso links. |
| man/shift.Rd | Updates source reference and improves docs; marks as internal. |
| man/plot.gg_vimp.Rd | Updates randomForestSRC reference text/URL. |
| man/plot.gg_variable.Rd | Clarifies return semantics and adds seealso/refs. |
| man/plot.gg_survival.Rd | Expands return description and adds seealso links. |
| man/plot.gg_roc.Rd | Clarifies input types, multiclass behavior, and examples; updates references. |
| man/plot.gg_rfsrc.Rd | Documents new notch parameter and expands behavior details. |
| man/plot.gg_error.Rd | Expands input/return documentation and references. |
| man/ggrandomforests.news.Rd | Adds generated Rd for ggrandomforests.news. |
| man/gg_variable.Rd | Clarifies gg_variable return object semantics; cleans seealso. |
| man/gg_survival.Rd | Improves argument/value documentation for survival estimators. |
| man/gg_roc.rfsrc.Rd | Improves gg_roc documentation; adds default oob = TRUE. |
| man/gg_rfsrc.rfsrc.Rd | Expands gg_rfsrc docs (by/conf.int/surv_type/return shapes). |
| man/gg_partial_rfsrc.Rd | Clarifies gg_partial_rfsrc interface, return value, and seealso. |
| man/gg_error.Rd | Updates references/seealso. |
| man/ggRandomForests-package.Rd | Updates package-level docs and randomForestSRC requirement/reference. |
| man/calc_roc.rfsrc.Rd | Expands calc_roc docs and return value details. |
| man/bootstrap_survival.Rd | Adds generated internal documentation for bootstrap_survival. |
| cran-comments.md | Updates CRAN submission notes for v2.7.0. |
| README.md | Major README refresh: installation, quick start, function reference, updated references. |
| R/utils.R | Introduces shared internal utilities: shift() and .label_strata(). |
| R/surv_partial.rfsrc.R | Improves roxygen docs; minor formatting/lintr adjustments. |
| R/plot.gg_vimp.R | Fixes 1:nvar bug via seq_len; fixes aes mappings using .data; refines arg handling. |
| R/plot.gg_variable.R | Replaces gather with pivot_longer; improves type handling; refactors plotting branches. |
| R/plot.gg_survival.R | Expands documentation and cleans exports. |
| R/plot.gg_roc.R | Fixes aes mappings, removes dead code, uses seq_len() for class iteration. |
| R/plot.gg_rfsrc.R | Adds notch parameter; fixes aes mappings; migrates to pivot_longer; adjusts survival grouping aesthetics. |
| R/plot.gg_error.R | Migrates to pivot_longer; fixes legend suppression for single-outcome; adds point-vs-line behavior. |
| R/nelson.R | Refactors stratification labeling via .label_strata; fixes life integral computation. |
| R/kaplan.R | Refactors stratification labeling via .label_strata; fixes life integral computation. |
| R/help.R | Updates package-level roxygen docs and references/requirements. |
| R/ggrandomforests.news.R | Adds roxygen + internal docs for ggrandomforests.news(). |
| R/gg_vimp.R | Fixes seq_len() issues; improves error messaging; modernizes tidyr usage; normalizes randomForest importance column. |
| R/gg_variable.R | Expands return documentation; uses seq_len(); minor lintr/robustness tweaks. |
| R/gg_survival.R | Improves documentation and minor code cleanup. |
| R/gg_roc.R | Adds defaults for oob; improves validation/error messages; documentation updates. |
| R/gg_rfsrc.R | Improves by-column validation; modernizes pivoting; refines binary-class handling; documents/ships bootstrap_survival. |
| R/gg_partial_rfsrc.R | Converts string “error returns” to stop(); refactors end-of-function splitting without dplyr pipes. |
| R/gg_partial.R | Lintr spacing; refactors categorical factor-level normalization. |
| R/gg_error.R | Improves seealso/docs; simplifies training arg parsing with list(...). |
| R/calc_roc.R | Adds .validate_which_outcome; fixes AUC trapezoid computation; moves shift() out to utils. |
| NEWS.md | Adds v2.7.0 release notes and retains older version sections. |
| NAMESPACE | Removes gather import; ensures pivot_longer import; trims explicit exports of methods. |
| DESCRIPTION | Bumps version/date; adds covr and vdiffr to Suggests; minor formatting cleanup. |
| CONTRIBUTING.md | Adds contributor guide including package conventions and workflows. |
| .lintr | Adds repository lintr configuration and exclusions. |
| .gitignore | Stops ignoring generated R package files; adds FUSE temp ignores. |
| .github/workflows/test-coverage.yaml | Updates Codecov action usage; adds env vars and caching tweaks. |
| .github/workflows/lint.yaml | Updates actions versions and makes lint failures fail CI. |
| .github/workflows/check-standard.yaml | Updates checkout action and adds caching/env updates. |
| .github/workflows/check-release.yaml | Updates checkout action and adds caching/env updates. |
| .github/workflows/R-CMD-check.yaml | Uses rtools 44, treats warnings as errors, adds caching, sets vdiffr env var. |
| .Rbuildignore | Ignores new meta files and .lintr; adds FUSE temp ignores. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| prob_col <- colnames(gg_dta)[1] | ||
| obs_col <- colnames(gg_dta)[2] | ||
| if (ncol(gg_dta) < 3) { | ||
| # Binary classification: single probability column + observed class |
There was a problem hiding this comment.
The binary-vs-multiclass branch selection is based on ncol(gg_dta) < 3, but gg_rfsrc() appends a group column when by is used. That makes a binary classification result have 3 columns (probability, y, group) and incorrectly routes into the multiclass code path. Consider detecting the number of probability columns explicitly (e.g., setdiff(names(gg_dta), c("y","group"))) rather than using ncol().
| } else { | ||
| # Multi-class: gather all class probability columns into long form | ||
| gathercols <- colnames(gg_dta)[-which(colnames(gg_dta) == "y")] | ||
| pivot_cols <- colnames(gg_dta)[-which(colnames(gg_dta) == "y")] |
There was a problem hiding this comment.
In the multiclass classification branch, pivot_cols excludes only y. If gg_dta includes group (from gg_rfsrc(..., by=...)), pivot_longer() will also pivot group into the probability long-form data, breaking the plot. Exclude all non-probability columns (at least y and group) from pivot_cols.
| pivot_cols <- colnames(gg_dta)[-which(colnames(gg_dta) == "y")] | |
| pivot_cols <- setdiff(colnames(gg_dta), c("y", "group")) |
| ) + | ||
| ggplot2::labs(y = "Predicted Value", x = colnames(gg_dta)[2]) + | ||
| ggplot2::theme( |
There was a problem hiding this comment.
The regression plot sets the x-axis title to colnames(gg_dta)[2], but the x aesthetic is either a constant 1 or the group column (when present). This labels the x-axis with the response variable name, which is misleading. Set the x-axis label to something appropriate (e.g. empty string or the grouping variable name) based on whether group is present.
| .label_strata <- function(tbl, data, by) { | ||
| tm_splits <- which(c(FALSE, sapply(seq(2L, nrow(tbl)), function(ind) { | ||
| tbl$time[ind] < tbl$time[ind - 1L] | ||
| }))) | ||
|
|
||
| # Use levels() for factors to respect the existing ordering; fall back to | ||
| # unique() (in order of first appearance) for character/numeric vectors. | ||
| by_col <- data[[by]] | ||
| lbls <- if (is.factor(by_col)) levels(by_col) else unique(by_col) | ||
|
|
||
| tbl$groups <- lbls[1L] | ||
| for (ind in seq(2L, length(tm_splits) + 1L)) { | ||
| tbl$groups[tm_splits[ind - 1L]:nrow(tbl)] <- lbls[ind] | ||
| } |
There was a problem hiding this comment.
.label_strata() can fail when there is only one stratum (no time resets) or when tbl has <2 rows: seq(2L, nrow(tbl)) and seq(2L, length(tm_splits)+1L) produce decreasing sequences (e.g. 2:1), leading to out-of-bounds indexing and tm_splits[ind-1] being NA. Add guards for nrow(tbl) < 2 and length(tm_splits) == 0 so the function safely returns all rows labeled with lbls[1] in these cases.
| # Add point/smooth layers for non-classification forests | ||
| if (family != "class") { | ||
| if (points) { | ||
| gg_plt <- ggplot2::ggplot( | ||
| gg_dta_mlt, | ||
| ggplot2::aes(x = .data$value, y = .data$yhat) | ||
| ) + | ||
| gg_plt <- gg_plt + | ||
| ggplot2::geom_point(...) | ||
| } else { | ||
| gg_plt <- ggplot2::ggplot( | ||
| gg_dta_mlt, | ||
| ggplot2::aes(x = .data$value, y = .data$yhat) | ||
| ) + | ||
| gg_plt <- gg_plt + | ||
| ggplot2::geom_smooth(...) | ||
| } |
There was a problem hiding this comment.
In the panel-plot branch for non-classification forests, the code builds gg_plt with geom_point() (or geom_boxplot()) and then unconditionally adds another geom_point()/geom_smooth() depending on points. This can duplicate layers and also makes points = FALSE ineffective (a point layer was already added). Consider constructing the ggplot with mappings only, then add exactly the requested geometry based on points/smooth.
| if (!nzchar(Sys.getenv("CI"))) { | ||
|
|
||
| ## ---- Shared fixtures ------------------------------------------------------- | ||
|
|
||
| # Classification — iris | ||
| local({ | ||
| set.seed(42L) | ||
| rfsrc_iris <- randomForestSRC::rfsrc( | ||
| Species ~ ., | ||
| data = iris, | ||
| importance = TRUE, | ||
| tree.err = TRUE, | ||
| ntree = 100L | ||
| ) | ||
|
|
||
| test_that("snapshot: gg_vimp classification", { | ||
| gg_dta <- gg_vimp(rfsrc_iris) | ||
| vdiffr::expect_doppelganger("gg_vimp classification rfsrc", plot(gg_dta)) | ||
| }) |
There was a problem hiding this comment.
This snapshot test file registers many vdiffr::expect_doppelganger() cases but there is no tests/testthat/_snaps/ directory committed in this PR. With CI unset (typical local runs and CRAN checks), these tests will run and should fail on a fresh checkout until snapshots are accepted/committed. Consider gating snapshots behind an explicit opt-in env var (e.g. VDIFFR_RUN_TESTS=true) or commit the _snaps/ baselines and run snapshots in CI.
… output; update snapshot test guard for better CI compatibility
This pull request introduces a new release (v2.7.0) for the
ggRandomForestspackage, focusing on critical bug fixes for plotting functions, improved linter and CI integration, documentation updates, and minor internal code improvements. The release addresses several long-standing issues withggplot2aesthetics, improves code quality enforcement, and updates dependencies and suggested packages.Bug fixes and plotting improvements:
plot.gg_rfsrcandplot.gg_rocmethods whereaes()used bare string literals, causing incorrect plot mappings; now uses.data[[col]]for proper variable mapping. Also removed dead code inplot.gg_rocand fixed legend suppression inplot.gg_errorfor single-outcome forests.gg_rfsrcandgg_partialfunctions, improving robustness for edge cases and categorical data handling.Testing and CI enhancements:
lint.yamlnow fails CI on any lint issue and uses updated actions.R-CMD-check.yamltreats warnings as errors, uses Rtools 44, and adds stricter build arguments. [1] [2]test-coverage.yamlremoves duplicate codecov uploads and updates to v5. [1] [2]Code quality and linter configuration:
.lintrconfiguration file to relax line length, allow multiple naming conventions, and set exclusions for legacy files.gg_partial.Documentation and package metadata:
DESCRIPTIONfor version 2.7.0, addedcovrandvdiffrtoSuggests, and improved documentation for several functions. [1] [2] [3].Rbuildignoreto include new files for code review and CI.Internal code improvements:
gg_errorand related methods for clarity and robustness. [1] [2] [3]calc_auc, and factored out outcome normalization incalc_roc. [1] [2] [3]These changes collectively improve the reliability, maintainability, and usability of the package, especially for users relying on its plotting and CI workflows.